stats: Add a mechanism to turn a string to a StatName on the hot-path without taking a lock.#7008
stats: Add a mechanism to turn a string to a StatName on the hot-path without taking a lock.#7008jmarantz wants to merge 11 commits intoenvoyproxy:masterfrom
Conversation
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
|
This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
stale
bot
added
the
stale
stale
bot
removed
the
stale
|
Sorry -- I put this together a while back to get comments but I think I'll take it out of draft status. I'm not sure if this is worth it -- seeking opinions. I just came across another situation where I think we might need this GRPC methods get injected as a token into a stat-name, and IIUC these are arbitrary tokens, but in any given application there will only be a finite number of them. So having a lock-free way to get StatNames for those may be needed. |
Signed-off-by: Joshua Marantz <jmarantz@google.com>
|
/retest |
|
🔨 rebuilding |
mattklein123
left a comment
mattklein123
left a comment
There was a problem hiding this comment.
@jmarantz this is very cool, but my opinion is that we should put this on ice until we see perf traces that make us think we should do this. I'm hesitant to make the stat code more complicated than it needs to be. WDYT?
/wait-any
|
agreed. this adds complexity without explicit evidence its needed. But I think we might get it when I convert the gRPC stats to use StatName. Let's hold off on this till that review is ready to go (still working on it but haven't had time this week to make progress). |
|
This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
stale
bot
added
the
stale
|
This pull request has been automatically closed because it has not had activity in the last 14 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
2 participants
Description: In some cases, a stat name string cannot be determined until requests are being processed. See #7003 for more details. In this case, we want to spend some memory for TLS cache infrastructure to allow conversion of that string to a StatName without taking a lock (after the first lookup). This PR provides
StringStatNameMapandScope::fastMemoryIntensiveStatNameLookupto try to solve this problem. The awkward wording of the lookup method is intended to discourage its use for stats where the name can be symbolized during construction. This should only be used when the name is based on tokens that are discoverable only at runtime, e.g. due to stat-name prefix from request headers.Risk Level: medium
Testing: //test/...
Docs Changes: n/a
Release Notes: n/a
Fixes: #7003